Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coreos-secex-ignition-prepare: remount /usr rw if needed #3241

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

nikita-dubrovskii
Copy link
Contributor

@nikita-dubrovskii nikita-dubrovskii commented Nov 5, 2024

Fedora 41 comes with systemd-256, where /usr is read-only during initramfs time.

See similar issue description in coreos/ignition#1891

@dustymabe
Copy link
Member

Issue: coreos/ignition#1891

hmm. I don't think this commit message should link to this issue in this way. Yes, it is the same underlying cause, but this PR isn't fixing that issue. I think here we should link to the upstream systemd change and then at the bottom of the commit message we can say

See similar issue description in coreos/ignition#1891

Fedora 41 comes with systemd-256, where /usr is read-only during initramfs time.

@nikita-dubrovskii
Copy link
Contributor Author

hmm. I don't think this commit message should link to this issue in this way.

Done

dustymabe
dustymabe previously approved these changes Nov 5, 2024
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dustymabe
Copy link
Member

actually. now that I think about it a bit more maybe the right approach is to actually set ProtectSystem= to a different value than the new default in the service instead:

https://github.com/coreos/fedora-coreos-config/blob/357e33f2bba5d6d7175c803373327a9f5b736538/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-secex-ignition-prepare.service

did you consider that?

@nikita-dubrovskii
Copy link
Contributor Author

actually. now that I think about it a bit more maybe the right approach is to actually set ProtectSystem= to a different value than the new default in the service instead:

https://github.com/coreos/fedora-coreos-config/blob/357e33f2bba5d6d7175c803373327a9f5b736538/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-secex-ignition-prepare.service

did you consider that?

No, but setting it to false feels more insecure than rw. I guess now it makes sense to start working on fixing issue itself (and than dropping workaround here).

@dustymabe
Copy link
Member

I guess now it makes sense to start working on fixing issue itself

oh? is there a more complete fix?

@nikita-dubrovskii
Copy link
Contributor Author

oh? is there a more complete fix?

At least for secex i could patch Ignition, so it uses 01-secex.ign from /etc/../../ or smth like this. Need to think more about that, this unit is not the only one affected by systemd-256

@jlebon
Copy link
Member

jlebon commented Nov 5, 2024

This is OK and is similar to the workaround in #3031, but you'll want to add MountFlags=slave so that we're not remounting rw for everyone.

actually. now that I think about it a bit more maybe the right approach is to actually set ProtectSystem= to a different value than the new default in the service instead:

I don't think that'll work. It's not that systemd sets ProtectSystem= by default on all units and that's why they see /usr being read-only. The knob, which was named the same as the unit setting, is for systemd itself to mount /usr read-only. If you e.g. cosa run -c --kargs 'rd.break', you can see that /usr is mounted read-only.

Fedora 41 comes with systemd-256, where /usr is read-only during
initramfs time.

See similar issue description in coreos/ignition#1891
@jlebon jlebon enabled auto-merge (rebase) November 6, 2024 21:27
@jlebon jlebon merged commit c762dbc into coreos:testing-devel Nov 6, 2024
3 checks passed
@nikita-dubrovskii nikita-dubrovskii deleted the issues_1891 branch November 7, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants